Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the GossipEthTxPool to eliminate race conditions and improve code safety. The main goal is to make the subscription to the mempool synchronous at initialization while keeping the event handling asynchronous, and to correct the subscription parameters to maintain proper bloom filter invariants.
Key changes:
- Moved mempool subscription from asynchronous
Subscribe()to synchronous initialization inNewGossipEthTxPool - Renamed
Subscribe()toUpdateBloomFilter()to better reflect its actual purpose - Fixed subscription parameter from
falsetotrueto maintain bloom filter invariants
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| graft/coreth/plugin/evm/vm.go | Updated function call from Subscribe to UpdateBloomFilter |
| graft/coreth/plugin/evm/gossip_test.go | Replaced test subscription logic with direct UpdateBloomFilter call, removing race-prone IsSubscribed check |
| graft/coreth/plugin/evm/eth_gossiper.go | Refactored to move subscription to constructor, renamed method, removed testing-only IsSubscribed, added channel close handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // UpdateBloomFilter continuously listens for new pending transactions from the | ||
| // mempool and adds them to the bloom filter. If the bloom filter reaches its | ||
| // capacity, it is reset and all pending transactions are re-added. | ||
| func (g *GossipEthTxPool) UpdateBloomFilter(ctx context.Context) { |
There was a problem hiding this comment.
nit: maybe can be renamedBloomFilterUpdateLoop or DispatchBloomUpdater etc etc to indicate the continous loop.
| case pendingTxs := <-g.pendingTxs: | ||
| case pendingTxs, ok := <-g.pendingTxs: | ||
| if !ok { | ||
| log.Debug("pending txs channel closed, shutting down subscription") |
There was a problem hiding this comment.
nit: I don't think this channel closes normally, should this be a warn/error log?
JonathanOppenheimer
added
coreth
|
This PR has become stale because it has been open for 30 days with no activity. Adding the |
4 participants
Why this should be merged
IsSubscribedis a testing only function that really indicates that the implementation is inherently racy. The subscription should be synchronous while the handling of the subscription should be async.SubscribeTransactionscurrently takes infalse. This isn't a bug, because the bool is ignored for the legacy pool... But if it weren't ignored, then this would violate the invariant that the bloom filter is a superset of the inner set. So now it is set totrue.How this works
imo, the code would be cleaner if there was a
CloseorShutdownfunction on the mempool with the goroutine started inNewGossipEthTxPool... But that seemed to be a bigger refactor involving node shutdown.How this was tested
existing CI
Need to be documented in RELEASES.md?
no